Repository navigation
fix(subagent): start agents on windows from a stored launch command - #864
Conversation
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: Windows CLI executable resolution and the relevant test regressions must be fixed before merge.
I reviewed the full diff and the affected launch, probe, manifest-resolution, and compatibility call paths. I also checked the branch history and PR description; the repository rules in AGENTS.md/CLAUDE.md and CONTEXT-MAP.md; backward compatibility for stored CLI rows and cross-shaped commands; test changes for weakening/skips; and the inner-layer import direction. The three concrete issues are inline.
Verification:
PYTHONPATH=.:plugins-dist/everos-memory:plugins-dist/design-engine:plugins-dist/ppt-engine uv run pytest -n 0 tests/test_utils_commands.py tests/test_subagent_vendored_agents.py::TestAWindowsShapedCommand::test_a_missing_windows_launcher_disables_the_row_with_the_token_named tests/test_subagent_vendored_agents.py::TestASpaceInTheResolvedPath::test_resolution_quotes_a_spaced_subagent_dir -q-> 11 passed, 2 failed.PYTHONPATH=.:plugins-dist/everos-memory:plugins-dist/design-engine:plugins-dist/ppt-engine uv run pytest -n 0 tests/test_subagent_probe.py -q-> 66 passed.
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: the three existing Windows launch findings remain unresolved.
This revision only reformats one assignment in vendored_agents.py; it does not change the CLI executable resolution, cross-shaped command parsing, or the Windows-parser test setup raised in the existing threads. I reviewed the delta and rechecked it against the full diff, affected callers, repository rules and architecture, backward compatibility, and test changes. No new finding was introduced by the formatting commit.
Focused verification on this head:
PYTHONPATH=.:plugins-dist/everos-memory:plugins-dist/design-engine:plugins-dist/ppt-engine uv run pytest -n 0 tests/test_utils_commands.py tests/test_subagent_vendored_agents.py::TestAWindowsShapedCommand::test_a_missing_windows_launcher_disables_the_row_with_the_token_named tests/test_subagent_vendored_agents.py::TestASpaceInTheResolvedPath::test_resolution_quotes_a_spaced_subagent_dir -q -> 11 passed, 2 failed, with the same two failures already reported.
59c0955 to
1820ec4
Compare
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: the three open Windows launch findings remain unresolved.
The tree is identical to the previous revision; only the second commit message changed from style(subagent) to refactor(subagent). I rechecked the full diff, affected callers, repository rules and architecture, backward compatibility, and test changes. The commit-message correction introduces no new code finding, but it does not address the open findings.
Focused verification:
PYTHONPATH=.:plugins-dist/everos-memory:plugins-dist/design-engine:plugins-dist/ppt-engine uv run pytest -n 0 tests/test_utils_commands.py tests/test_subagent_vendored_agents.py::TestAWindowsShapedCommand::test_a_missing_windows_launcher_disables_the_row_with_the_token_named tests/test_subagent_vendored_agents.py::TestASpaceInTheResolvedPath::test_resolution_quotes_a_spaced_subagent_dir -q -> 11 passed, 2 failed, unchanged.
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; suggestions only, and they are marked inline.
The two reproduced regressions are fixed, and I withdrew the remaining prior blocker because it was pre-existing and has an explicit-path workaround. I reviewed the new delta and full PR diff, affected launch/probe callers and history, AGENTS.md/CLAUDE.md/CONTEXT-MAP.md rules, backward compatibility, test-strength changes, and the inner-layer import direction. One narrow POSIX spaced-path readiness edge is marked inline as a follow-up; it does not meet the fourth-round blocking bar because it needs a spaced product root plus a missing launcher and can be escaped by relocating the product.
Verification:
PYTHONPATH=.:plugins-dist/everos-memory:plugins-dist/design-engine:plugins-dist/ppt-engine uv run pytest -n 0 tests/test_utils_commands.py tests/test_subagent_vendored_agents.py tests/test_subagent_probe.py -q -> 180 passed.
|
Not a blocker. Four more findings from the same pass, none of which holds the merge Severity of this one finding, not a verdict on the pull request. The blocking findings from this pass are review threads on the changed files; GitHub renders those collapsed, as a file name with no text. Gates on the merged tree: 1 --
|
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; suggestions only, and they are marked inline.
The POSIX quoted-root defect is fixed on this revision. command_tokens now groups both quote forms, and the added end-to-end tests drive discover_product_rows over a spaced root with both a missing and present launcher, covering the fail-closed and ready outcomes. I reviewed the delta against the full PR, affected callers and history, repository rules and architecture, backward compatibility, and test strength. I found no new blocker. The previously noted bare-name Windows CLI .cmd behavior remains a pre-existing follow-up with an explicit-path workaround.
Verification:
PYTHONPATH=.:plugins-dist/everos-memory:plugins-dist/design-engine:plugins-dist/ppt-engine uv run pytest -n 0 tests/test_utils_commands.py tests/test_subagent_vendored_agents.py tests/test_subagent_probe.py -q -> 185 passed.
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; suggestions only, and they are marked inline.
The shared resolver makes discovery, registration, and newly generated installers agree, and I reviewed the delta against the full PR, affected callers/history, repository rules and architecture, backward compatibility, and test strength. One incomplete consumer remains inline: the default scaffold smoke still whitespace-splits the now-quoted command. I am carrying it as a nonblocking follow-up under the late-round bar because --no-smoke provides a working path through creation/registration, although the default workflow and four existing CLI tests currently fail.
Verification:
PYTHONPATH=.:plugins-dist/everos-memory:plugins-dist/design-engine:plugins-dist/ppt-engine uv run pytest -n 0 tests/test_utils_commands.py tests/test_subagent_vendored_agents.py tests/test_subagent_probe.py tests/test_cli_agents_commands.py tests/test_cli_subagent_setup.py -q -> 298 passed, 4 failed, 1 skipped. The four failures are the whitespace-policy CLI cases named inline.
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; suggestions only, and they are marked inline.
This revision updates the CLI assertions to the new quoted-path success contract, but the previously reported nonblocking smoke follow-up remains unresolved: the default smoke still raw-splits quoted commands. The spaced-home and --here cases therefore still write the scaffold and exit nonzero. The SUBAGENT_PYTHON case now also expects success from a deliberately nonexistent interpreter, which readiness correctly rejects. I reviewed the test-only delta against the production flow, affected callers, repository rules and architecture, backward compatibility, and test strength; no new finding beyond the already recorded follow-up emerged.
Verification:
PYTHONPATH=.:plugins-dist/everos-memory:plugins-dist/design-engine:plugins-dist/ppt-engine uv run pytest -n 0 tests/test_utils_commands.py tests/test_subagent_vendored_agents.py tests/test_subagent_probe.py tests/test_cli_agents_commands.py tests/test_cli_subagent_setup.py -q -> 299 passed, 3 failed, 1 skipped.
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: the new spaced-path tests must assert the discovery-only contract or request registration.
The production smoke fix is correct: it now parses the stored command with command_argv, and the spaced-root invocations reach exit code 0. I reviewed the delta against the full PR, affected CLI/discovery callers and history, repository rules and architecture, backward compatibility, and test strength. The one new finding is inline.
This meets the late-round blocking bar: this revision introduces the failing assertions; every relevant Linux test run reaches them; and the branch has no passing-test path until the tests either add --register or inspect the discovered row.
Verification:
PYTHONPATH=.:plugins-dist/everos-memory:plugins-dist/design-engine:plugins-dist/ppt-engine uv run pytest -n 0 tests/test_utils_commands.py tests/test_subagent_vendored_agents.py tests/test_subagent_probe.py tests/test_cli_agents_commands.py tests/test_cli_subagent_setup.py -q -> 299 passed, 3 failed, 1 skipped.
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; suggestions only, and they are marked inline.
The registration-contract blocker is fixed: the three spaced-path tests now opt into registration before reading the pinned roster, and the production smoke path continues to parse stored commands through command_argv. I reviewed the delta and the full PR diff, relevant callers and history, repository rules and architecture constraints, backward compatibility, and whether the tests were weakened. The pre-existing bare-name Windows CLI .cmd resolution limitation remains a follow-up with an explicit executable path as a working escape hatch.
Verification: PYTHONPATH=.:plugins-dist/everos-memory:plugins-dist/design-engine:plugins-dist/ppt-engine uv run pytest -n 0 tests/test_utils_commands.py tests/test_subagent_vendored_agents.py tests/test_subagent_probe.py tests/test_cli_agents_commands.py tests/test_cli_subagent_setup.py -q (302 passed, 1 skipped); git diff --check github/main...HEAD (clean).
The probe alone split a command with posix=False on Windows, while
AcpClient.launch and the cli backend kept POSIX shlex.split. The two
then disagreed in both directions: the probe found the interpreter of
an unquoted {PYTHON} command that the launch still mangled into
C:Users...python.exe, and it kept the quotes shlex.quote puts round
host_raven_acp_command's path, so it reported missing a program the
launch starts.
One parser for the probe and both launchers is what #864 adds, so the
probe goes back to the split the launchers use rather than gaining a
second Windows rule here.
Co-authored-by: Claude (claude-opus-5-5[1m]) <noreply@anthropic.com>
…login shell (#880) ## Summary On Windows the sub-agents page and the launches read the environment the gateway started with, so an agent installed after it started stayed "not on the login shell PATH" until a restart. This reads the PATH a freshly opened terminal would get from the registry, takes it again on Check again, Connect and Test, and starts a bare program from it. - `login_shell_env` and `refresh_login_shell_env` both go through `_capture_now`: the registry on Windows, the login shell elsewhere. A refresh on Windows used to look for a login shell, find none, and keep the first capture. - `_capture_windows` returns raven's own environment with only `PATH` rebuilt: the machine and user stores' `Path`, `%VAR%` references expanded, followed by the live PATH. The stores' other values are not copied over raven's: they are what Windows builds an environment from (`ComSpec` and `TEMP` unexpanded, the machine's `USERNAME=SYSTEM`). - The capture keeps the upper-case names `os.environ` has on Windows, which is the spelling `probe._login_path` and the other readers ask for, so an agent already on the gateway's PATH stays found beside the ones the stores add. - A stored reference expands with raven's own values first (they are this logon's, and the child carries them), then user over machine for a variable an installer added after raven started. - `resolve_program`: CreateProcess looks a bare name up on the gateway's own PATH and never on the env block it is handed, so on Windows the acp launch, the cli launch and the Kimi Code ask look it up on the child's PATH instead, by CreateProcess's own rule that a name with no extension means `.exe`. Only an `.exe` or `.com` is put in: CreateProcess never turns a bare name into a `.cmd` or `.bat`, and doing it here would put a cli prompt through cmd.exe's parser. - On Windows no shell is driven, including the Git for Windows bash that `SHELL` can name there: its MSYS environment carries a `:`-joined POSIX PATH and no `SystemRoot`. Left out on purpose: splitting the command line. The probe keeps splitting a command the way both launchers do; a Windows rule for the probe alone (`posix=False`) keeps the quotes `shlex.quote` and a Program Files path carry and disagrees with the launch. #864 routes the probe and the launchers through one parser (`raven/utils/commands.py`), so the backslash mangling of a `{PYTHON}` command is closed there. Once both land, #864's `launch_argv` (a PATHEXT lookup on the gateway's PATH) and `resolve_program` here want to become one lookup on the child's PATH; whichever lands second folds them. ## Type - [x] Fix - [ ] Feature - [ ] Docs - [ ] CI / tooling - [ ] Refactor - [ ] Other ## Verification - `python -m pytest tests/test_subagent_host_env.py tests/test_subagent_probe.py tests/test_subagent_third_party.py tests/test_subagent_kimi_code.py tests/test_rpc_subagents.py tests/test_cli_agents_commands.py -q` (project venv, all extras, this tree on `PYTHONPATH`): 620 passed, 1 skipped. - Full suite, `python -m pytest -q` the same way: 27683 passed, 119 skipped, 8 failed. Seven fail on this host at main too: five `test_config_update_providers.py` proxy cases that read the host's proxy variables, `test_subagent_node_runtime.py::test_what_cannot_be_read_names_nothing` under root, and `test_install_script.py::test_resolve_node_dir_answers_each_case_it_exists_for` with an npm on PATH. - The eighth, `test_subagent_kimi_code.py::test_a_kimi_that_does_not_know_acp_is_told_to_upgrade`, is a load race at main as well: with the stand-in forced to exit before the host's first `_send`, main and this branch both answer "stdin is closed" with no remedy. - Mutation check: 13 single-construct mutants (refresh routing, the PATH spelling, the expansion order, the overlay, the union with the live PATH, folding store names, matching references without case, each of the three launch call sites, the `.exe` rule, the `.cmd` refusal, the first capture's platform switch), each caught by the test written for it. - Windows is simulated on Linux: a fake `winreg` holding the stock store values (REG_EXPAND_SZ unexpanded), `os.environ` with upper-case names, and `;` as the path separator for the probe test. Not run on a Windows host. - `ruff check` and `ruff format --check` over CI's targets, `lint-imports` (10 kept, 0 broken), `ty check` on the touched modules, and `scripts/check_large_files.py` and `scripts/check_source_language.py` over the change: all clean. - [x] Relevant tests pass locally - [x] Relevant lint / type checks pass locally - [ ] User-facing docs or screenshots are updated when needed ## Risk Windows only. On POSIX `_capture_now` is `_capture` and `resolve_program` returns the argv it was given, so behavior matches main. On Windows a child gets raven's environment with PATH rebuilt registry-first, and a bare program found as an `.exe` or `.com` on that PATH starts from there; anything else is left to CreateProcess's own search, as before. Not changed here: a bare name that exists only as a `.cmd` shim (npx and other npm installs) still does not start on Windows, and a `%PATH%` reference inside a stored Path stays unexpanded. Rollback: revert the squash commit. - [x] Security impact considered - [x] Backward compatibility considered - [x] Rollback path is clear for risky changes ## Related Issues #864 --------- Co-authored-by: Claude (claude-opus-5-5[1m]) <noreply@anthropic.com>
7e4256f to
51191e7
Compare
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; suggestions only, and they are marked inline.
The new revision fixes the Windows single-quote failure: host_raven_acp_command() and its forwarded --config path now use the same platform quoter as the launch parser. The added tests cover sibling and PATH launchers, spaced and unspaced paths, both simulated platforms, and config forwarding through command_argv. Range-diff confirms the prior eight reviewed commits are unchanged rebases and this is the only substantive delta.
I covered the repository rules, full PR diff and new delta, callers and history, backward compatibility, architecture constraints, and whether tests were weakened. The pre-existing bare-name Windows CLI .cmd resolution limitation remains only a follow-up with an explicit executable path as a working escape hatch.
Verification: PYTHONPATH=.:plugins-dist/everos-memory:plugins-dist/design-engine:plugins-dist/ppt-engine uv run pytest -n 0 tests/test_utils_commands.py tests/test_subagent_builtin_agents.py tests/test_subagent_vendored_agents.py tests/test_subagent_probe.py tests/test_cli_agents_commands.py tests/test_cli_subagent_setup.py -q (342 passed, 1 skipped); git diff --check github/main...HEAD (clean).
51191e7 to
7ce8b10
Compare
|
Answering the board note above, item by item, at
From the stated-not-filed list, #880's lookup is folded in: the ACP launch looks a bare program up once, on the child's PATH, directory by directory with Also in this push:
Verification and the mutation check are in the description. |
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; suggestions only, and they are marked inline.
The nine follow-up commits are sound. Windows quoting now round-trips backslashes and embedded quotes; ACP program lookup uses the child's PATH and permits batch shims without extending that exposure to prompt-bearing CLI launches; shipped and generated installers use the shared resolver while retaining their older-Raven fallback. The newly posted resolutions of the POSIX probe and host-ACP quoting failures match the current implementation and tests.
I covered the repository rules, full diff and revision delta, callers and history, backward compatibility, architecture constraints, and whether tests were weakened. All threads I opened are resolved. The pre-existing bare-name Windows CLI .cmd limitation remains only a follow-up with an explicit executable path as a working escape hatch.
Verification: PYTHONPATH=.:plugins-dist/everos-memory:plugins-dist/design-engine:plugins-dist/ppt-engine uv run pytest -n 0 tests/test_utils_commands.py tests/test_subagent_builtin_agents.py tests/test_subagent_vendored_agents.py tests/test_subagent_probe.py tests/test_subagent_host_env.py tests/test_subagent_kimi_code.py tests/test_subagent_third_party.py tests/test_subagent_acp.py tests/test_cli_agents_commands.py tests/test_cli_subagent_setup.py -q (998 passed); git diff --check github/main...HEAD (clean).
|
Not a blocker -- from the re-acceptance pass on head 7ce8b10 (merged onto main 289426c), over the fix commits that came after the first pass. Severity of this one finding, not a verdict on the pull request. The blocking findings from this pass are review threads on the changed files; GitHub renders those collapsed, as a file name with no text.
Checked on the merged tree: the machine gates pass. The acceptance pass also ran the full suite: nothing red because of this PR; the reds are the known environment-only ones plus one process-group timing test that also fails on main under load. |
7ce8b10 to
144669d
Compare
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; suggestions only, and they are marked inline.
The two new fixes close the re-acceptance notes without weakening coverage. command_tokens now honors host escape rules so literal quotes cannot hide a following launcher, while preserving cross-shaped absolute paths for fail-closed readiness checks. Shipped and generated installers running under an older Raven now reject spaced command paths before registration, but still accept a spaced cwd when the command does not interpolate it.
I covered the repository rules, full diff and revision delta, callers and history, backward compatibility, architecture constraints, and test strength. All threads I opened remain resolved. The pre-existing bare-name Windows CLI .cmd limitation remains only a follow-up with an explicit executable path as a working escape hatch.
Verification: PYTHONPATH=.:plugins-dist/everos-memory:plugins-dist/design-engine:plugins-dist/ppt-engine uv run pytest -n 0 tests/test_utils_commands.py tests/test_subagent_builtin_agents.py tests/test_subagent_vendored_agents.py tests/test_subagent_probe.py tests/test_subagent_host_env.py tests/test_subagent_kimi_code.py tests/test_subagent_third_party.py tests/test_subagent_acp.py tests/test_cli_agents_commands.py tests/test_cli_subagent_setup.py -q (1042 passed); git diff --check github/main...HEAD (clean).
|
Not a blocker -- three items from the re-acceptance pass on head 144669d (merged onto main 6126965). Nothing blocking was found, and the touched tests pass (262 passed across tests/test_utils_commands.py, tests/test_subagent_vendored_agents.py and tests/test_cli_agents_commands.py). Severity of this one finding, not a verdict on the pull request. The blocking findings from this pass are review threads on the changed files; GitHub renders those collapsed, as a file name with no text.
|
A subagent row keeps its launch as one string, and where that string is
split decided whether the spawn ever saw what was written. Every launch
and probe reached for POSIX-mode shlex.split on any host, so on Windows
the backslashes every interpreter and launcher path carries were eaten
by the escape rules before CreateProcess ran: C:\Users\..\python.exe
reached the spawn as C:Users..python.exe, and a bare npx was handed to
a launcher that only finds npx.cmd by full path.
Add raven/utils/commands as the one interpretation. command_argv reads a
stored command by CommandLineToArgvW's own backslash and quote rules on
Windows (shlex elsewhere), command_quote produces a token that survives
that split, and launch_argv adds the PATHEXT resolution of a bare
extensionless argv[0]. AcpClient.launch, the cli backend, the codex
dialect and the probes all parse through it, and the vendored manifests
quote {PYTHON} and {SUBAGENT_DIR} when they resolve. A product whose
interpreter sits under C:\Program Files now starts instead of failing
with a mangled path.
Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
The launcher probes judge the command a row holds, not spawn it, but they tokenised through the spawn split. On POSIX that split is shlex, which eats the backslashes of a Windows drive path: C:\gone\python.exe reached _is_absolute_path as C:gonepython.exe and read as no absolute path at all, so a Windows-shaped product whose launcher was gone read as ready on a POSIX scan -- the fail-closed check the class docstring promises. Add command_tokens for the judging arm: split on whitespace and double quotes but keep every other byte, so a Windows token keeps its shape and a quoted one stays whole, on any host. The spawn arm still parses by the host's own rules in command_argv. Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
command_tokens grouped double quotes only, but command_quote emits POSIX paths under shlex.quote's single quotes. A product whose tree or interpreter path needs quoting (a spaced root, a paren) then produced a launcher token that began with an apostrophe, so _is_absolute_path called it relative, _launcher_missing checked nothing, and a manifest whose run.py was gone reached the roster enabled=True -- the fail-open the readiness gate exists to refuse. A reviewer reproduced it end to end through discover_product_rows with a control and a base control. Group single quotes as well as double in _split_shape, and drive the agreement through discover_product_rows over a spaced root: a missing launcher stays disabled, a present one stays ready. Windows already read a single quote as a literal byte, so this changes only the POSIX half. Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
Discovery quoted the manifest placeholders, but raven agents new refused a whitespace path outright and install.py exited with one, while _register_row substituted with no quoting at all -- so the same folder registered three different ways depending on which door wrote the row. The comment agents new carried said quoting was a seam it must not open, which the discovery half had already opened. Resolve all three through resolve_subagent_command: the interpreter and agent root are quoted with command_quote when the field is split back into argv (command / resumeCommand) and left plain for cwd, and every producer calls it. Shipped manifests already use a forward slash, so the quoted root and its launcher stay one token to either parser. The two whitespace refusals go away -- a spaced install path or interpreter now registers and actually starts, matching what discovery lists. Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
Four agents-new and installer tests pinned the whitespace refusal this PR removes: with the placeholder quoting unified, a spaced home, working directory, or SUBAGENT_PYTHON scaffolds and registers with the path quoted into the command rather than being refused. The assertions follow the new behaviour -- the command tokenises to the launcher path and interpreter as single argv entries. The collection of this suite is Linux-only in CI (os.geteuid), so these run there. Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
The smoke check split its command on bare whitespace, so a spaced interpreter or root -- now quoted into the roster command -- reached Popen as fragments and the handshake failed with "can't open file '...space'". Run the smoke through command_argv so it spawns exactly the argv a dispatch would. The spaced-path cli assertions use real symlinked interpreters and read the command through command_argv rather than a prefix match, so they assert the parsed argv on any host. Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
The three spaced-path assertions read the registered row, but plain raven agents new only scaffolds the folder -- the row lands only with --register -- so get_agents returned nothing and the unpack failed. Pass --register (and keep --no-smoke off, so the smoke covers the spaced launch these tests exist to prove). Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
Use the shared command quoter for the host Raven executable and its forwarded config path so the Windows parser receives the original argv. Cover sibling and PATH launchers on both platforms, including spaced paths, and assert config forwarding through the production parser. Co-authored-by: Codex <noreply@openai.com>
command_quote's Windows arm wrapped a token in double quotes and escaped only an inner quote. CommandLineToArgvW halves a run of backslashes wherever a quote follows it, the closing quote included, so a token ending in one backslash came back as an unbalanced-quote error, a token ending in two lost one without a word, and a backslash in front of an inner quote closed the quoted run early. The docstring promised a round trip for any token; SUBAGENT_PYTHON is the route a trailing backslash takes in today. Every run of backslashes in front of a quote is now doubled. The round trip is asserted over a battery of tokens on both arms, at either end of the line, and the splitter's docstring names the two places it is narrower than the real parser. Co-authored-by: Claude (claude-opus-5-5[1m]) <noreply@anthropic.com>
The acp launch resolved a bare program name twice. launch_argv searched the gateway's own PATH with PATHEXT, so npx found npx.cmd, and resolve_program then searched the child's PATH for an .exe only. The first answer won whenever it found the name, so an acp server that is a batch file and was installed after raven started, which the refreshed capture and the probe both find, still could not start, and one that sits in two places started from wherever the gateway's PATH pointed. The two are now one lookup on the child's PATH. resolve_program takes batch_files: the acp launch passes it, because an acp server's argv is configuration and its turns travel over stdio, so a .bat or .cmd may stand in for the program there. The cli launch and the kimi ask leave it off, since their argv carries the prompt and a batch file hands its arguments to cmd.exe's parser. PATH directories are searched in order with .exe first within each, the order a terminal finds the name in. launch_argv and its gateway-PATH lookup are gone. The guard that keeps a path or an unstartable suffix from being looked up is asserted with a lookup that answers every name; the old test's lookup answered none, so the guard could be deleted with the file green. Co-authored-by: Claude (claude-opus-5-5[1m]) <noreply@anthropic.com>
_is_absolute_path's docstring still said its tokens came from str.split(), so a path with spaces arrived in fragments, and that the manifests keep their paths space-free to avoid it. Neither has held since both launcher probes moved to command_tokens and the row producers started quoting, and the paragraph was the stated reason the shape judgment is safe, so a reader trusting it reasoned from the wrong tokeniser. Co-authored-by: Claude (claude-opus-5-5[1m]) <noreply@anthropic.com>
resolve_subagent_command says discovery, raven agents new --register and each folder's install.py all write through it, and the five shipped installers still substituted their paths unquoted. A folder whose path has a space then pinned a row whose command splits that path into two arguments, so the pinned row and the discovered row for one folder disagreed. The shipped installers now write through the same rule. A folder can outlive the raven that shipped it, and no release so far (v0.2.0 through v0.2.4) has raven.utils.commands, so under an older raven the installer writes the unquoted row it always wrote instead of failing on the import. Co-authored-by: Claude (claude-opus-5-5[1m]) <noreply@anthropic.com>
The scaffold's install.py imports raven.utils.commands, which no release so far has (v0.2.0 through v0.2.4). A folder raven agents new generates is never refreshed when raven changes version, so after a downgrade its installer failed on that import before writing anything. It now falls back to the unquoted row, the shape an older raven reads. Its other raven call, add_third_party_subagent, is in every release. Co-authored-by: Claude (claude-opus-5-5[1m]) <noreply@anthropic.com>
raven agents new no longer refuses a spaced landing or interpreter path, and four texts still described the refusal: _resolved_python's docstring called itself the guard's resolver, the c12 section header listed the guard, a test's name and docstring said a clean SUBAGENT_PYTHON passes the gate, and the smoke test said its stand-in had to be a file because the smoke split a command with str.split. The smoke splits with command_argv now, and each text says what its code still does. Co-authored-by: Claude (claude-opus-5-5[1m]) <noreply@anthropic.com>
TestResolveSubagentCommand says each host arm is exercised whichever host the suite runs on, and its Windows round trip skipped unless the splitter it read was already the Windows one, which on CI it never is. Both round trips now swap the commands module's own platform read, the way the splitter's tests do, so the Windows one runs on Linux and fails when the Windows quoting breaks. Co-authored-by: Claude (claude-opus-5-5[1m]) <noreply@anthropic.com>
With batch_files a name that already carries .cmd or .bat is looked up on the child's PATH like a bare one, and nothing asserted it: limiting the named-suffix check back to .exe and .com left all 661 tests in the launch suites green. The lookup-guard test now resolves npx.cmd beside npx, and that mutant fails it. Co-authored-by: Claude (claude-opus-5-5[1m]) <noreply@anthropic.com>
test_launcher_is_gone_reads_a_quoted_spaced_path_as_one_token named an absolute launcher that does not exist, so a whitespace split called the row gone as well, and the test passed whichever tokeniser _launcher_is_gone used. Its launcher now exists, which leaves the quoted, spaced interpreter as the only missing file; putting the whitespace split back fails the test. Co-authored-by: Claude (claude-opus-5-5[1m]) <noreply@anthropic.com>
Read host escape rules without losing foreign absolute path shapes. Keep literal and escaped quotes in arguments from hiding the launcher that follows them, including an unmatched leading apostrophe on Windows. Cover discovery, stale rows, quote round trips, drive paths and UNC paths. Co-authored-by: Codex <noreply@openai.com>
Reject whitespace in substituted command paths before registration when an older Raven lacks the shared quoting resolver. Apply the same rule to the scaffold and all five shipped installers, preserving plain paths and working directories that are not part of the command. Exercise both refusals and supported fallback cases. Keep the Windows test collection guard and compare launcher paths by their native meaning. Co-authored-by: Codex <noreply@openai.com>
Mark a token started when reading a backslash so an argument consisting only of backslashes or an escaped quote survives at the end of a command and before another argument. Pin the round trip through list2cmdline. Co-authored-by: Codex <noreply@openai.com>
Recognize CR and LF in POSIX readiness checks, matching the launch parser while retaining Windows separators and quoted whitespace. Cover discovery and stale rows with both missing and present launchers. Co-authored-by: Codex <noreply@openai.com>
Describe the cross-platform readiness check using the domain term for an agent found on disk. Co-authored-by: Codex <noreply@openai.com>
144669d to
4a1e1e6
Compare
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; suggestions only, and they are marked inline.
The three follow-up commits correctly close the latest re-acceptance notes. The Windows parser now retains arguments made only of backslashes or escaped quotes; readiness parsing uses the same CR/LF separators as POSIX shlex while preserving Windows space/tab behavior; and the documentation now uses the canonical discovered-agent terminology. The new tests exercise both launch and readiness tokenizers across both platforms without weakening prior coverage.
I covered the repository rules, full diff and revision delta, callers and history, backward compatibility, architecture constraints, and test strength. All threads I opened remain resolved. The pre-existing bare-name Windows CLI .cmd limitation remains only a follow-up with an explicit executable path as a working escape hatch.
Verification: PYTHONPATH=.:plugins-dist/everos-memory:plugins-dist/design-engine:plugins-dist/ppt-engine uv run pytest -n 0 tests/test_utils_commands.py tests/test_subagent_builtin_agents.py tests/test_subagent_vendored_agents.py tests/test_subagent_probe.py tests/test_subagent_host_env.py tests/test_subagent_kimi_code.py tests/test_subagent_third_party.py tests/test_subagent_acp.py tests/test_cli_agents_commands.py tests/test_cli_subagent_setup.py -q (1076 passed); git diff --check github/main...HEAD (clean).
Decode stored single-quoted absolute path tokens on Windows without changing adjacent native arguments or literal apostrophes. Use the same decoder for launch and readiness, including path values after equals. Cover real CLI execution, ACP spawn arguments, availability probes, and existing or missing launchers alongside native quoting controls. Co-authored-by: Codex <noreply@openai.com>
Describe the shared compatibility policy and qualify the probe fixture's literal-apostrophe explanation after accepting legacy path quotes. Co-authored-by: Codex <noreply@openai.com>
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; suggestions only, and they are marked inline.
The substantive delta correctly preserves legacy Windows rows whose absolute paths were stored with POSIX single quotes, while leaving ordinary apostrophes literal and retaining native Windows parsing for neighboring arguments. The compatibility decoder is shared by launch and readiness paths, and the added coverage reaches probes, vendored readiness, third-party launches, and child-environment resolution. The documentation-only follow-up accurately narrows the stated contract.
I covered the repository rules, full diff and revision delta, callers and history, backward compatibility, architecture constraints, and test strength. All threads I opened remain resolved. The pre-existing bare-name Windows CLI .cmd limitation remains only a follow-up with an explicit executable path as a working escape hatch.
Verification: PYTHONPATH=.:plugins-dist/everos-memory:plugins-dist/design-engine:plugins-dist/ppt-engine uv run pytest -n 0 tests/test_utils_commands.py tests/test_subagent_builtin_agents.py tests/test_subagent_vendored_agents.py tests/test_subagent_probe.py tests/test_subagent_host_env.py tests/test_subagent_kimi_code.py tests/test_subagent_third_party.py tests/test_subagent_acp.py tests/test_cli_agents_commands.py tests/test_cli_subagent_setup.py -q (1101 passed); git diff --check github/main...HEAD (clean).
ZuyiZhou
left a comment
ZuyiZhou
left a comment
There was a problem hiding this comment.
LGTM. Read the full diff and call sites; CI is green and the tests exercise the fix.
4 participants
Summary
Stored Windows launch commands were parsed with POSIX rules, which removed backslashes from unquoted interpreter and launcher paths. Launch consumers now share platform-aware parsing and quoting, while retaining compatibility with single-quoted absolute paths saved by older callers.
=and the quote concatenation emitted for apostrophes inside a path. Decode only that token so neighboring native Windows arguments keep their backslashes and literal apostrophes. Launch and readiness use the same compatibility decoder.Type
Verification
Checked on native Windows using an existing uv environment, with the source imported from the reviewed checkout and
PYTHONUTF8=1.Result: 292 passed. This covers a real CLI child launched through the backend, ACP spawn arguments, CLI/ACP availability probes, and existing/missing launcher checks, with both legacy and native quoting controls.
The final 25 added cases were also run in an isolated process with the previously published parser: 12 failed and 13 passed. Four isolated mutations were caught: disabling compatibility (8 failures), removing the launch quote guard (1), removing the readiness quote guard (1), and using POSIX parsing for the entire Windows command (15). A seeded 50,000-case check through the native producer and both consumers reported no argv or readiness-token mismatches.
Whole-branch checks:
All passed: 24 Python files passed Ruff checks; production type checks passed; all 10 import contracts were kept. The pinned commitlint CLI also passed over the full branch range. The final documentation-only follow-up changes no executable behavior.
Limits: a broader Windows selection stopped after 283 passing tests at
test_cli_probe_reports_the_resolved_absolute_path, whose bare POSIX shell fixture is not resolved as a Windows executable. This is not counted as a passing suite. The ACP regression checks the arguments delivered to process creation; it does not claim to verify the existing POSIX-only process-group shutdown path on Windows. The local Docker daemon was unavailable, so no new local Linux or macOS run is claimed. The full repository suite and a live WebUI dispatch were not run locally for this revision.The pre-submit sweep covered the complete branch diff, all command producers and consumers, native/legacy quoting boundaries, installer fallbacks, failure paths, test strength, layer contracts, repository gates, and these disclosed verification limits.
No screenshot update applies. Parser docstrings describe the legacy-path compatibility and how to preserve literal quotes.
Risk
Compatibility applies to single-quoted absolute path tokens, including path values after
=; it does not enable general POSIX shell syntax on Windows. Non-path apostrophes remain literal. To preserve literal single quotes around path-looking data, quote the entire argument with native double quotes. Existing stored path tokens are read compatibly without rewriting configuration; newly generated commands keep native quoting.POSIX launch parsing is unchanged. Readiness preserves foreign absolute-path spellings and checks launcher existence; it is not a general command validator. Older installers reject paths their Raven version cannot represent before changing the roster.
ACP batch shims execute configured server arguments through cmd.exe. Prompt-bearing CLI and Kimi calls do not opt into batch lookup. A CLI agent installed only as an npm shim still needs an explicit executable path. Reverting the squash commit restores the prior behavior.
Related Issues
#880